Expose transaction_depth through get_transaction_depth() method
#3427
Conversation
mpyw
force-pushed
the
feat/get-transaction-depth
branch
2 times, most recently
from
e924285
to
a1e92a6
Compare
There was a problem hiding this comment.
I'm not a fan of the trait split. The SQLite driver can be refactored a bit to track the transaction depth in the connection struct itself (or the ConnectionWorker struct), we've just got to be more careful of keeping the value in-sync.
Instead of a bespoke trait, since you're only implementing it directly for the connection types anyway, the method can just go on the Connection trait.
It can be a provided method that forwards to a method on TransactionManager. It's not a breaking change to add a required method to the latter because it's #[doc(hidden)] and thus exempt from our semver guarantees.
Since most users would likely just do conn.get_transaction_depth() != 0 to check if the connection is in a transaction, I would also add a provided method like
#[inline]
fn is_in_transaction(&self) -> bool {
self.get_transaction_depth() != 0
}No emoji in the commit messages, please. It's not necessary, and who knows what it might break.
mpyw
force-pushed
the
feat/get-transaction-depth
branch
from
a1e92a6
to
4f9d3f9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I now understand what you mean by provided method. It means "default implementation" in other languages. |
mpyw
force-pushed
the
feat/get-transaction-depth
branch
9 times, most recently
from
6ab39ce
to
48dbba7
Compare
SQLite implementation is currently WIP
mpyw
force-pushed
the
feat/get-transaction-depth
branch
from
48dbba7
to
8f45019
Compare
This comment was marked as outdated.
This comment was marked as outdated.
mpyw
force-pushed
the
feat/get-transaction-depth
branch
2 times, most recently
from
3cdccde
to
3453955
Compare
mpyw
force-pushed
the
feat/get-transaction-depth
branch
from
3453955
to
10d0aea
Compare
I have included `AtomicUsize` in `WorkerSharedState`. Ideally, it is not desirable to execute `load` and `fetch_add` in two separate steps, but we decided to allow it here since there is only one thread writing. To prevent writing from other threads, the field itself was made private, and a getter method was provided with `pub(crate)`.
|
@abonander Strictly speaking, it seems more correct to use [Edit] I am considering adopting |
mpyw
force-pushed
the
feat/get-transaction-depth
branch
from
7356c48
to
54c73b6
Compare
|
@abonander If we were to replace the implementation with diff --git a/sqlx-sqlite/src/connection/worker.rs b/sqlx-sqlite/src/connection/worker.rs
index 6dda814a..d36b1df2 100644
--- a/sqlx-sqlite/src/connection/worker.rs
+++ b/sqlx-sqlite/src/connection/worker.rs
@@ -34,14 +34,14 @@ pub(crate) struct ConnectionWorker {
}
pub(crate) struct WorkerSharedState {
- transaction_depth: AtomicUsize,
+ transaction_depth: parking_lot::RwLock<usize>,
cached_statements_size: AtomicUsize,
pub(crate) conn: Mutex<ConnectionState>,
}
impl WorkerSharedState {
pub(crate) fn get_transaction_depth(&self) -> usize {
- self.transaction_depth.load(Ordering::Acquire)
+ *self.transaction_depth.read()
}
pub(crate) fn get_cached_statements_size(&self) -> usize {
@@ -104,7 +104,7 @@ impl ConnectionWorker {
};
let shared = Arc::new(WorkerSharedState {
- transaction_depth: AtomicUsize::new(0),
+ transaction_depth: parking_lot::RwLock::new(0),
cached_statements_size: AtomicUsize::new(0),
// note: must be fair because in `Command::UnlockDb` we unlock the mutex
// and then immediately try to relock it; an unfair mutex would immediately
@@ -193,12 +193,12 @@ impl ConnectionWorker {
update_cached_statements_size(&conn, &shared.cached_statements_size);
}
Command::Begin { tx } => {
- let depth = shared.transaction_depth.load(Ordering::Acquire);
+ let mut depth = shared.transaction_depth.write();
let res =
conn.handle
- .exec(begin_ansi_transaction_sql(depth))
+ .exec(begin_ansi_transaction_sql(*depth))
.map(|_| {
- shared.transaction_depth.fetch_add(1, Ordering::Release);
+ *depth += 1;
});
let res_ok = res.is_ok();
@@ -209,9 +209,9 @@ impl ConnectionWorker {
// immediately otherwise it would remain started forever.
if let Err(error) = conn
.handle
- .exec(rollback_ansi_transaction_sql(depth + 1))
+ .exec(rollback_ansi_transaction_sql(*depth + 1))
.map(|_| {
- shared.transaction_depth.fetch_sub(1, Ordering::Release);
+ *depth -= 1;
})
{
// The rollback failed. To prevent leaving the connection
@@ -223,13 +223,13 @@ impl ConnectionWorker {
}
}
Command::Commit { tx } => {
- let depth = shared.transaction_depth.load(Ordering::Acquire);
+ let mut depth = shared.transaction_depth.write();
- let res = if depth > 0 {
+ let res = if *depth > 0 {
conn.handle
- .exec(commit_ansi_transaction_sql(depth))
+ .exec(commit_ansi_transaction_sql(*depth))
.map(|_| {
- shared.transaction_depth.fetch_sub(1, Ordering::Release);
+ *depth -= 1;
})
} else {
Ok(())
@@ -249,13 +249,13 @@ impl ConnectionWorker {
continue;
}
- let depth = shared.transaction_depth.load(Ordering::Acquire);
+ let mut depth = shared.transaction_depth.write();
- let res = if depth > 0 {
+ let res = if *depth > 0 {
conn.handle
- .exec(rollback_ansi_transaction_sql(depth))
+ .exec(rollback_ansi_transaction_sql(*depth))
.map(|_| {
- shared.transaction_depth.fetch_sub(1, Ordering::Release);
+ *depth -= 1;
})
} else {
Ok(())This approach would ensure that operations on |
Issue
transaction_depthwith a Read-Only Getter Method #3426Description
This PR introduces the
get_transaction_depth()method for exposing the current transaction depth in various database connections.MySqlConnectionandPgConnectionnow implement theTransactionDepth::get_transaction_depth()method to synchronously retrieve the transaction depth.SqliteConnectionimplements theAsyncTransactionDepth::get_transaction_depth()method to retrieve the transaction depth asynchronously by acquiring a lock (as the existing implementation requires a lock to access the transaction level).AnyConnectionuses theAsyncTransactionDepth::get_transaction_depth()method to handleSqliteConnection, and in thedyn AnyConnectionBackendimplementation, it dispatches to the respective connection's implementation.Concerns
get_transaction_depth()is used for both traits, there is a potential for import collisions between the two traits. However, it is unlikely that both traits would be imported simultaneously, so this should not pose a significant issue.SqliteConnectionimplementation, I'm unsure if this approach is optimal. Specifically, is it acceptable to acquire a lock just to retrieve the transaction nesting level? Additionally, my understanding is that the lock is automatically released when it goes out of scope—is this correct?Additional Context
I am relatively new to using Rust professionally, having only recently started working with it. If there are any issues with my approach or if I am violating any best practices, please feel free to point them out.